Repository navigation
Conversation
…s not allowed anymore
github-actions
Bot
added
the
t-tooling
apify-service-account
added
the
tested
|
✅ The link checker did not find any broken links. See more at https://github.com/apify/apify-client-js/actions/runs/42833494809#summary-129889504430 |
mvolfik
changed the title
mvolfik
added
the
t-core-services
apify-factory
left a comment
apify-factory
left a comment
There was a problem hiding this comment.
Looks good, nothing blocking.
🏭 Factory review — ℹ️ not auto-approved
This change was reviewed but cannot be auto-approved: a gate failed, so the review cannot end in an approval.
| check | result |
|---|---|
| Static gates | ❌ 15/16 — no-risky-content |
| Review findings | ✅ none |
| Reviewer escalation | ✅ none |
Juror 1 — claude-sonnet-5-5, adversarial |
⏭️ skipped — a gate failed |
Why it was not auto-approved
Failed gates:
no-risky-content: test/integration/_fixtures.ts: environment variable access (env-access); test/integration/run.test.ts: environment variable access (env-access)
The comments above are the review itself; this section only explains the approval decision. Push a change and request @apify-factory again to re-run it, or ask a human to review.
Reviewed a2d32a881 · workflow run
vdusek
changed the title
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
most of them is overlap with Python counterpart
| (rec) => rec !== undefined, | ||
| { timeoutSecs: 120, backoffFactor: 2 }, | ||
| ); | ||
| const runTokenClient = makeClient(record!.value as string); |
There was a problem hiding this comment.
provide a clear failure when the token is missing
Suggestion: pollUntilCondition returns undefined on timeout and does not throw. If the Actor never stores TOKEN, this line fails with "Cannot read properties of undefined". An explicit assertion gives a clear message:
| const runTokenClient = makeClient(record!.value as string); | |
| expect(record, 'the Actor did not store its run token').toBeDefined(); | |
| const runTokenClient = makeClient(record!.value as string); |
✍️ Drafted by Claude Code
| message: 'Hello from metamorph!', | ||
| }); | ||
| expect(metamorphedRun.id).toBe(run.id); | ||
| await client.build((await actorClient.build('0.0')).id).waitForFinish(); |
There was a problem hiding this comment.
we should check the build status, same as in Python
Suggestion: check the build status, as the iterateDatasetItems() test does. A failed build then fails here, and not later in start() with a less clear error.
| await client.build((await actorClient.build('0.0')).id).waitForFinish(); | |
| const build = await client.build((await actorClient.build('0.0')).id).waitForFinish(); | |
| expect(build.status).toBe('SUCCEEDED'); |
✍️ Drafted by Claude Code
| }); | ||
| expect(metamorphedRun.id).toBe(run.id); | ||
| await client.build((await actorClient.build('0.0')).id).waitForFinish(); | ||
| const run = await actorClient.start(); |
There was a problem hiding this comment.
shouldn't we add some limits to this run?
Suggestion: this run sleeps for 60 s with the default memory, on an account that parallel tests share. Limits like the other custom Actor test in this file would keep it small and stop it if the cleanup fails:
| const run = await actorClient.start(); | |
| const run = await actorClient.start(undefined, { memory: 256, runTimeoutSecs: 120 }); |
✍️ Drafted by Claude Code
| // hello-world can finish before the metamorph lands; the API call was still exercised. | ||
| rethrowUnlessAlreadyFinished(err); | ||
| } | ||
| const metamorphedRun = await runTokenClient.run(run.id).metamorph(HELLO_WORLD_ACTOR); |
There was a problem hiding this comment.
keep test coverage of the metamorph input body
Nit: the old test sent an input to metamorph(). Without it, no integration test sends a metamorph input body.
| const metamorphedRun = await runTokenClient.run(run.id).metamorph(HELLO_WORLD_ACTOR); | |
| const metamorphedRun = await runTokenClient.run(run.id).metamorph(HELLO_WORLD_ACTOR, { message: 'Hello from metamorph!' }); |
✍️ Drafted by Claude Code
| test('metamorph() transforms a started run into another Actor, keeping the run id', async () => { | ||
| const run = await client.actor(HELLO_WORLD_ACTOR).start(); | ||
| const runClient = client.run(run.id); | ||
| // Metamorphing the run from outside is not allowed, so we cheat a bit: the Actor stores its own run token in its key-value store, |
There was a problem hiding this comment.
Nit: this comment and the one at line 281 say the same thing, and the first line is longer than 120 chars. One comment here is enough, and line 281 can go:
| test('metamorph() transforms a started run into another Actor, keeping the run id', async () => { | |
| const run = await client.actor(HELLO_WORLD_ACTOR).start(); | |
| const runClient = client.run(run.id); | |
| // Metamorphing the run from outside is not allowed, so we cheat a bit: the Actor stores its own run token in its key-value store, | |
| // Only the run's own token may metamorph it, so the Actor stores its token in its KVS for the test to read. |
✍️ Drafted by Claude Code
fix for breaking change from apify/apify-core#32513